Fixes for HD-Audio DMA stopping #3167

ranj063 · 2021-09-21T17:20:30Z

The recommendation for HD-Audio DMA stopping programming sequence is to stop/pause the DMA in the FW after the host has cleared the RUN bit.

The FW PR thesofproject/sof#4844 changes the sequence to not stop the host and link DMA during STOP trigger IPC. The host DMA is stopped when the PCM_FREE IPC which is after the host clears the RUN bit.
For link DMA, the host uses the DAI_CONFIG IPC to stop the DMA which is sent after the RUN bit is cleared.
For more detailed info on the programming sequence, please refer to: thesofproject/sof@f9a9802

bardliao · 2021-09-22T05:55:28Z

sound/soc/sof/intel/hda-stream.c

 	return 0;
 }

+int hda_dsp_stream_reset(struct snd_sof_dev *sdev, struct hdac_stream *hstream)


On commit message, "This would mean that we leave them decoupled during system suspend." Should this sentence be more specific to when the pcm is active?

@ranj063 I'd also extend the commit a bit. "This would mean that
we leave them decoupled during system suspend." I think we should clearly indicate why (hw spec, causes failures, etc).

sound/soc/sof/intel/hda-stream.c

sound/soc/sof/pcm.c

ujfalusi · 2021-09-23T08:02:50Z

sound/soc/sof/pm.c

+	if (ret < 0)
+		dev_warn(sdev->dev, "snd_sof_dma_trace_release failed with error: %d\n", ret);
+
 suspend:


I'm not super excited about the spreading around of dtrace related calls, this is certainly going to make the SOF client support a no go..

Quoting the commit message for comment:

The recommended programming sequence for STOP trigger for normal PCM streams is as follows: 1. Send the STOP_TRIGGER IPC to the FW 2. Stop the host DMA 3. Send the PCM_FREE IPC to do the DMA reset 4. Finally, perform stream reset and coupling of host/link DMAs. But due to the lack of STOP and PCM_FREE IPC for trace, this sequence is modified as follows: 1. Stop the host DMA for trace 2. Send the CTX_SAVE IPC to the FW for DMA reset 3. Perform stream reset and host/link DMA coupling in snd_sof_dma_trace_release().

The current sequence is:

In `snd_sof_release_trace()` 1. Send the STOP_TRIGGER IPC to the FW (`snd_sof_dma_trace_trigger()`) 2. Stop the host DMA (`snd_sof_dma_trace_release()`) in `sof_suspend()` 3. Send the CTX_SAVE IPC to the FW for DMA reset

The correct place for the 'Send the PCM_FREE IPC to do the DMA reset' and 'Finally, perform stream reset and coupling of host/link DMAs.' is within the snd_sof_dma_trace_release() so the sequence would be:

In `snd_sof_release_trace()` 1. Send the STOP_TRIGGER IPC to the FW (`snd_sof_dma_trace_trigger()`) 2. Stop the host DMA (`snd_sof_dma_trace_release()`) 3. Send the PCM_FREE IPC to do the DMA reset (`snd_sof_dma_trace_release()`) 4. Finally, perform stream reset and coupling of host/link DMAs. (`snd_sof_dma_trace_release()`) in `sof_suspend()` 5. Send the CTX_SAVE IPC

Without moving dtrace related calls all around the place.

Other thing I don't really see is how this will conflict with #3099?
It is doing almost similar thing, but in a different way.
Do you for example need to do all 4 steps for s0ix?

@ujfalusi I guess the problem is the lack of the IPCs for dma-trace. But this will indeed complicate auxbus plus how we keep the DSP neutral. OTOH, the SOF interface should be a superset that has enough flexibility to support all vendors, so I think it's ok to extend the interface, but this needs to be generic enough.

@kv2019i, @ranj063, correct me if I'm wrong, but the dma-trace on the firmware side is stopped/paused/off in response to the SOF_IPC_PM_GATE message if the BIT(4) is set (SOF_PM_NO_TRACE / HDA_PM_NO_DMA_TRACE).
The sequence proposed by this PR does not change the order regarding to dtrace DMA, it just moves the CTX_SAVE a bit earlier.

In light of this, the current sequence is:

In `snd_sof_release_trace()` 1. Send the STOP_TRIGGER IPC to the FW (`snd_sof_dma_trace_trigger()`) 2. Stop the host DMA (`snd_sof_dma_trace_release()`) 3. Send the CTX_SAVE IPC to the FW (unrelated to DMA) 4. Send SOF_IPC_PM_GATE to stop the DMA trace

and the presented sequence is:

1. Send the STOP_TRIGGER IPC to the FW (`snd_sof_dma_trace_trigger()`) 2. Send the CTX_SAVE IPC to the FW (unrelated to DMA) 3. Stop the host DMA (`snd_sof_dma_trace_release()`) 4. Send SOF_IPC_PM_GATE to stop the DMA trace

The issue boils down to a simple fact that SOF_IPC_PM_GATE is tasked to stop the DMA trace along with other unrelated things.
But If we first send SOF_IPC_PM_GATE with only BIT(4) set in flags (and if the pm_runtime_enable() is not refcounted on the firmware side) then we would only disable the trace and that could be embedded inside of the dtrace code itself to contain the code within it's scope.

keyonjie · 2021-09-23T08:12:12Z

sound/soc/sof/intel/hda-stream.c

 	return 0;
 }

+int hda_dsp_stream_reset(struct snd_sof_dev *sdev, struct hdac_stream *hstream)


It is actually a reset toggling you are doing here, not reset operation we should take at "RESET STREAM FLOW from PAUSED"?

On the other hand, I don't see a not from @plbossart 's email about the note "the proper HDA programming recommendation is to do a stream reset before coupling the host and link DMA's".

If hw_free() is called each time a stream is stopped, we should already reset the stream there, do you mean that hw_free() is not necessary called sometimes, so we have to perform an extra reset before a subsequent hw_params()?

The logic could be much clear to me if it goes like this:

set SRST=1 (not a 0->1->0 toggle) at initialization.

perform RESET->PAUSED transition at hw_params(), set SRST=0.

perform PAUSED->RUNNING transition at trigger_start, set RUN=1.

....

perform RUNNING->PAUSED transition at trigger stop via IPC, set RUN=0.

perform PAUSED->RESET transition at the subsequent hw_free(), set SRST=0.

With this logic, we don't need explicit SRST toggling at all, no?

kv2019i

A great find and fix. For merging, some work is needed to hide the HDaudio specifics sufficiently (plus if possible, minimize complications for auxbus-based trace clients).

kv2019i · 2021-09-23T17:08:29Z

sound/soc/sof/intel/hda-stream.c

 	return 0;
 }

+int hda_dsp_stream_reset(struct snd_sof_dev *sdev, struct hdac_stream *hstream)


@ranj063 I'd also extend the commit a bit. "This would mean that
we leave them decoupled during system suspend." I think we should clearly indicate why (hw spec, causes failures, etc).

sound/soc/sof/intel/hda-stream.c

sound/soc/sof/pcm.c

kv2019i · 2021-09-23T18:00:04Z

sound/soc/sof/intel/hda-trace.c

+
+		/* couple host and link DMA if link DMA channel is idle */
+		snd_sof_dsp_update_bits(sdev, HDA_DSP_PP_BAR,	SOF_HDA_REG_PP_PPCTL, mask, 0);
+


The commit touches generic code, so the "recommended programming sequence" seems wrong.

kv2019i · 2021-09-23T18:14:46Z

sound/soc/sof/pm.c

+	if (ret < 0)
+		dev_warn(sdev->dev, "snd_sof_dma_trace_release failed with error: %d\n", ret);
+
 suspend:


@ujfalusi I guess the problem is the lack of the IPCs for dma-trace. But this will indeed complicate auxbus plus how we keep the DSP neutral. OTOH, the SOF interface should be a superset that has enough flexibility to support all vendors, so I think it's ok to extend the interface, but this needs to be generic enough.

ujfalusi · 2021-09-30T12:30:02Z

@ranj063, what about the probes support? Is that also needs a revisit?

Would it make sense to split the PR to correct the audio (PCM) sequence and a separate one for dtrace and possibly for probes?

kv2019i

Only a few minor comments left.

sound/soc/sof/pcm.c

plbossart

I vote for merging and stress-tests on this PR, so that we can add fixups as needed and send upstream for the next kernel cycle.

kv2019i

I'll take back one comment (sorry!) and one commit message improvement.

sound/soc/sof/pcm.c

kv2019i · 2021-10-19T19:05:53Z

I vote for merging and stress-tests on this PR, so that we can add fixups as needed and send upstream for the next kernel cycle.

This is ok for me. My remaining concerns are non-functional comments (won't affect stress-tests), so feel free to merge. I already approved the FW one.

snd_sof_pcm_platform_hw_params() will be called when the stream is restarted with a prepare ioctl. This happens in two cases i.e. when a suspended stream is resumed or or when a stream is restarted without intermediate call to sof_pcm_hw_free(). Make sure to call snd_sof_pcm_platform_hw_free() in both these cases to keep it balanced. Signed-off-by: Ranjani Sridharan <[email protected]>

Paused streams must be stopped and platform hw_free should be invoked during system suspend so they can be restarted properly after system resume. Signed-off-by: Ranjani Sridharan <[email protected]>

Add a helper function to free PCM in the FW, stop the DMA and free the widget list. These actions are performed both during PCM trigger STOP and when a paused stream is freed during system suspend. Signed-off-by: Ranjani Sridharan <[email protected]>

Move the check for the prepared flag inside snd_pcm_dsp_pcm_free() to avoid having to check it before every invocation of the function. Signed-off-by: Ranjani Sridharan <[email protected]>

Even though the order of stopping the DMA and freeing the widget list is not important, align the sequence to match with the stop trigger to avoid confusion. Signed-off-by: Ranjani Sridharan <[email protected]>

Some DAI components, such as HDaudio, need to be stopped in two steps a) stop the DAI component b) stop the DAI DMA This patch enables this two-step stop by expanding the DAI_CONFIG IPC flags and split them into 2 parts. The 4 LSB bits indicate when the DAI_CONFIG IPC is sent, ex: hw_params, hw_free or pause. The 4 MSB bits are used as the quirk flags to be used along with the command flags. The quirk flag called SOF_DAI_CONFIG_FLAGS_2_STEP_STOP shall be set along with the HW_PARAMS command flag, i.e. before the pipeline is started so that the stop/pause trigger op in the FW can take the appropriate action to either perform/skip the DMA stop. If set, the DMA stop will be executed when the DAI_CONFIG IPC is sent during hw_free. In the case of pause, DMA pause will be handled when the DAI_CONFIG IPC is sent with the PAUSE command flag. Along with this, modify the signature for the hda_ctrl_dai_widget_setup/ hda_ctrl_dai_widget_free() functions to take additional flags as an argument and modify all users to pass the appropriate quirk flags. Only the HDA DAI's need to pass the SOF_DAI_CONFIG_FLAGS_2_STEP_STOP quirk flag during hw_params to indicate that it supports two-step stop and pause. Signed-off-by: Ranjani Sridharan <[email protected]>

For HDA DAI's the DMA must be paused after the RUN bit is cleared by the host. So, send the DAI_CONFIG IPC with just the SOF_DAI_CONFIG_FLAGS_PAUSE flag set to indicate this to the firmware. Signed-off-by: Ranjani Sridharan <[email protected]>

kv2019i

Thanks @ranj063 , looks good now!

ujfalusi

@ranj063, looks great, I have one comment regarding to the placement of the cancel_work_sync(), I can not see the discussion on it but I see that @plbossart marked it as resolved, so it should be fine from my side as well.

ujfalusi · 2021-10-20T07:21:27Z

sound/soc/sof/pcm.c

+	if (ret < 0)
+		err = ret;
+
+	cancel_work_sync(&spcm->stream[substream->stream].period_elapsed_work);


SO what was the conclusion on the cancel_work_sync() location? It is still the last step.

sound/soc/sof/intel/hda.c

plbossart · 2021-10-26T00:04:51Z

SOFCI TEST

cujomalainey · 2021-11-22T23:50:09Z

Just curious, is there a reason the ABI wasn't bumped for the kernel but it was for the FW?

ranj063 · 2021-11-22T23:55:07Z

Just curious, is there a reason the ABI wasn't bumped for the kernel but it was for the FW?

@cujomalainey we bump the ABI only for the FW always and handle the ABI requirements in the kernel if needed. In this particular case, there was no need to check for ABI because the new DAI_CONFIG IPC sent with older FW will simply be ignored.

kv2019i · 2021-11-24T11:26:24Z

@cujomalainey wrote:

Just curious, is there a reason the ABI wasn't bumped for the kernel but it was for the FW?

FYI, submitted #3296 to make this less confusing

Merge series from Kai Vehmanen <[email protected]>: Implement an updated programming sequence to handle DMA stop for Intel HD-Audio DMA. The new flow is only used if the firmware is sufficiently new to support the feature. SOF1.9.2 is the first release with the updated flow. The kernel changes are backwards compatible with old firmware releases. Likewise new firmware releases will work with old kernel. Series reviewed originally at: thesofproject/linux#3167

ranj063 requested review from bardliao, keyonjie, libinyang and lyakh as code owners September 21, 2021 17:20

ranj063 requested a review from plbossart September 21, 2021 17:20

ranj063 force-pushed the fix/hda_trigger branch 5 times, most recently from 2fb00cc to 15bc1d9 Compare September 21, 2021 18:26

ranj063 requested review from juimonen and kv2019i as code owners September 21, 2021 20:19

ranj063 force-pushed the fix/hda_trigger branch from c8e424b to 91f0f4e Compare September 22, 2021 01:03

This was referenced Sep 22, 2021

[BUG] DMA trigger stop failed when suspend-resume during capturing on CML-SKU0983-SDW thesofproject/sof#4779

Closed

Re-enable trace filtering and implement DMA reset op thesofproject/sof#4793

Closed

bardliao reviewed Sep 22, 2021

View reviewed changes

plbossart requested changes Sep 22, 2021

View reviewed changes

ujfalusi requested changes Sep 23, 2021

View reviewed changes

keyonjie reviewed Sep 23, 2021

View reviewed changes

ujfalusi mentioned this pull request Sep 23, 2021

ASoC: SOF: disable dma trace in s0ix #3099

Closed

kv2019i reviewed Sep 23, 2021

View reviewed changes

ranj063 force-pushed the fix/hda_trigger branch from 91f0f4e to f07d2b5 Compare October 5, 2021 05:47

ranj063 requested review from RanderWang, dbaluta, lgirdwood and singalsu as code owners October 5, 2021 05:47

ranj063 changed the title ~~ASoC: SOF: Intel: hda: couple host and link DMA during stop~~ Fixes for HD-Audio DMA stop/pause and trace DMA free Oct 5, 2021

ranj063 requested review from plbossart and ujfalusi October 5, 2021 05:51

ranj063 force-pushed the fix/hda_trigger branch from 2073783 to ad4ffd5 Compare October 19, 2021 02:23

kv2019i requested changes Oct 19, 2021

View reviewed changes

sound/soc/sof/pcm.c Show resolved Hide resolved

sound/soc/sof/pcm.c Outdated Show resolved Hide resolved

ranj063 force-pushed the fix/hda_trigger branch from ad4ffd5 to 59891b3 Compare October 19, 2021 17:05

plbossart previously approved these changes Oct 19, 2021

View reviewed changes

kv2019i requested changes Oct 19, 2021

View reviewed changes

sound/soc/sof/pcm.c Outdated Show resolved Hide resolved

sound/soc/sof/pcm.c Show resolved Hide resolved

ranj063 added 7 commits October 19, 2021 15:59

ASoC: SOF: call platform hw_free for paused streams during suspend

8725d4c

Paused streams must be stopped and platform hw_free should be invoked during system suspend so they can be restarted properly after system resume. Signed-off-by: Ranjani Sridharan <[email protected]>

ASoC: SOF: pcm: move the check for prepared flag

392c2eb

Move the check for the prepared flag inside snd_pcm_dsp_pcm_free() to avoid having to check it before every invocation of the function. Signed-off-by: Ranjani Sridharan <[email protected]>

ASoC: SOF: align the hw_free sequence with stop

905737f

Even though the order of stopping the DMA and freeing the widget list is not important, align the sequence to match with the stop trigger to avoid confusion. Signed-off-by: Ranjani Sridharan <[email protected]>

ranj063 dismissed plbossart’s stale review via 693860c October 19, 2021 23:00

ranj063 force-pushed the fix/hda_trigger branch from 59891b3 to 693860c Compare October 19, 2021 23:00

ranj063 requested review from bardliao, kv2019i and plbossart October 19, 2021 23:00

kv2019i approved these changes Oct 20, 2021

View reviewed changes

ujfalusi previously approved these changes Oct 20, 2021

View reviewed changes

plbossart dismissed ujfalusi’s stale review via 693860c October 20, 2021 16:09

plbossart approved these changes Oct 20, 2021

View reviewed changes

ranj063 merged commit 72af573 into thesofproject:topic/sof-dev Oct 26, 2021

kv2019i mentioned this pull request Nov 24, 2021

ASoC: SOF: rework FW ABI version handling on kernel side #3296

Closed

ranj063 deleted the fix/hda_trigger branch February 13, 2022 05:07


		/* couple host and link DMA if link DMA channel is idle */
		snd_sof_dsp_update_bits(sdev, HDA_DSP_PP_BAR, SOF_HDA_REG_PP_PPCTL, mask, 0);

Fixes for HD-Audio DMA stopping #3167

Fixes for HD-Audio DMA stopping #3167

Uh oh!

Conversation

ranj063 commented Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keyonjie Sep 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ujfalusi commented Sep 30, 2021

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kv2019i commented Oct 19, 2021

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

ujfalusi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

plbossart commented Oct 26, 2021

Uh oh!

cujomalainey commented Nov 22, 2021

Uh oh!

ranj063 commented Nov 22, 2021

Uh oh!

kv2019i commented Nov 24, 2021

Uh oh!

Reviewers

Assignees

Labels

ranj063 commented Sep 21, 2021 •

edited

Loading

keyonjie Sep 23, 2021 •

edited

Loading